Skip to content

n_nodes and number_of_nodes_xxx in tree related classes #40939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

n_nodes and number_of_nodes_xxx in tree related classes #40939

wants to merge 2 commits into from
+107 −99

Conversation

mantepse
Copy link
Contributor

@mantepse mantepse commented Sep 30, 2025 •
edited
Loading

fix #40935 in tree related classes.

cf #40932

Should number_of_nodes also be an alias, or perhaps instead of n_nodes?
Should n_nodes_to_the_right, n_nodes_at_depth also be aliases?

Does this pollute the namespace too much?

@mantepse mantepse requested a review from fchapoton September 30, 2025 19:20
This was referenced Sep 30, 2025
Merged
Merged
Merged
Merged
@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit db5657d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mantepse mantepse mentioned this pull request Oct 1, 2025
Closed
@dcoudert dcoudert mentioned this pull request Oct 1, 2025
Merged
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
c812284 
sagemathgh-40940: provide aliases for number_of_inversions and number_of_negative_ones
    
fix sagemath#40935 in ASM

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40940
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
040d852 
sagemathgh-40941: provide alias number_of_longest_increasing_subsequences
    
fix sagemath#40935 in permutation

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40941
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
fb6ecfe 
sagemathgh-40942: provide an alias number_of_relations
    
fix sagemath#40935 in posets

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40942
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
fbd5cda 
sagemathgh-40943: provide alias number_of_connected_components
    
fix sagemath#40935 for `connected_components`

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40943
Reported by: Martin Rubey
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
cb4dfda 
sagemathgh-40951: add methods for biconnected components
    
This PR do the following:
- add method `number_of_biconnected_components` to follow the proposal
of sagemath#40939
- add method `biconnected_components`
- move method `is_biconnected` from `graph.py` to `connectivity.pyx` and
expose it in `generic_graph.pyx`

The addition of method `biconnected_component_containing_vertex` is more
involved and deserves its own PR. Indeed, a cut vertex belongs to
multiple biconnected components.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40951
Reported by: David Coudert
Reviewer(s): Martin Rubey
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
7b42878 
sagemathgh-40940: provide aliases for number_of_inversions and number_of_negative_ones
    
fix sagemath#40935 in ASM

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40940
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
c44d57b 
sagemathgh-40941: provide alias number_of_longest_increasing_subsequences
    
fix sagemath#40935 in permutation

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40941
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
8a69f6a 
sagemathgh-40942: provide an alias number_of_relations
    
fix sagemath#40935 in posets

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40942
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
f7a8f20 
sagemathgh-40943: provide alias number_of_connected_components
    
fix sagemath#40935 for `connected_components`

as with sagemath#40939: we have to decide whether this does not pollute the name
space too much.
    
URL: sagemath#40943
Reported by: Martin Rubey
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 5, 2025
212ee66 
sagemathgh-40951: add methods for biconnected components
    
This PR do the following:
- add method `number_of_biconnected_components` to follow the proposal
of sagemath#40939
- add method `biconnected_components`
- move method `is_biconnected` from `graph.py` to `connectivity.pyx` and
expose it in `generic_graph.pyx`

The addition of method `biconnected_component_containing_vertex` is more
involved and deserves its own PR. Indeed, a cut vertex belongs to
multiple biconnected components.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40951
Reported by: David Coudert
Reviewer(s): Martin Rubey
@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2025

This is the last ticket concerning the transition to n_xxx or number_of_xxx. It would be good to finish this before the next release, for consistency.

@fchapoton
Copy link
Contributor

I am not happy with these changes. They make the methods less consistent than they were.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2025

Let me also note the following idea (I think I asked about it already once, but I cannot find the place anymore). We can hide aliases from tab-completion (or rather: produce aliases hidden from tab-completion) as follows:

class AlternatingSignMatrix(...):
    ...
    def __init__(self, parent, asm):
        ...
        self._matrix = asm
        self.__setattr__('number_negative_ones', self.number_of_negative_ones)
        Element.__init__(self, parent)
  
    def __dir__(self):
        return [f for f in super().__dir__() if f != "number_negative_ones"]

I am quite sure that we could also make this more automatic by having a class decorator or something similar. Do you see any downsides, @fchapoton?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchapoton fchapoton Awaiting requested review from fchapoton

Assignees
No one assigned
Labels
s: needs info
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

harmonize xxx_number
2 participants
@mantepse @fchapoton